Attributes rule based sampler #70
Conversation
| final AttributeKey<String> attributeKey; | ||
| final Sampler delegate; | ||
| final Pattern pattern; |
samplers/src/main/java/io/opentelemetry/contrib/samplers/SamplingRule.java
Outdated
Show resolved
Hide resolved
| final Sampler delegate; | ||
| final Pattern pattern; | ||
|
|
||
| SamplingRule(AttributeKey<String> attributeKey, String pattern, Sampler delegate) { |
There was a problem hiding this comment.
what would you think about only accepting pre-compiled Patterns here, so bad patterns are the responsibility of the user?
There was a problem hiding this comment.
The user is calling the builder so we would need to make the API change there. But we have a goal to optimize exact-match / prefix-match cases by not using Pattern (which Java Pattern itself should do, but doesn't...). I think String is fine
There was a problem hiding this comment.
Bad patterns are user's responsibility anyway
There was a problem hiding this comment.
Any thoughts to using Predicate<String> here? See: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/StringPredicates.java
samplers/src/main/java/io/opentelemetry/contrib/samplers/RuleBasedRoutingSampler.java
Outdated
Show resolved
Hide resolved
samplers/src/main/java/io/opentelemetry/contrib/samplers/RuleBasedRoutingSampler.java
Outdated
Show resolved
Hide resolved
samplers/src/main/java/io/opentelemetry/contrib/samplers/RuleBasedRoutingSampler.java
Outdated
Show resolved
Hide resolved
samplers/src/main/java/io/opentelemetry/contrib/samplers/RuleBasedRoutingSampler.java
Outdated
Show resolved
Hide resolved
samplers/src/main/java/io/opentelemetry/contrib/samplers/RuleBasedRoutingSamplerBuilder.java
Outdated
Show resolved
Hide resolved
samplers/src/main/java/io/opentelemetry/contrib/samplers/RuleBasedRoutingSamplerBuilder.java
Outdated
Show resolved
Hide resolved
samplers/src/main/java/io/opentelemetry/contrib/samplers/SamplingRule.java
Outdated
Show resolved
Hide resolved
| final Sampler delegate; | ||
| final Pattern pattern; | ||
|
|
||
| SamplingRule(AttributeKey<String> attributeKey, String pattern, Sampler delegate) { |
There was a problem hiding this comment.
The user is calling the builder so we would need to make the API change there. But we have a goal to optimize exact-match / prefix-match cases by not using Pattern (which Java Pattern itself should do, but doesn't...). I think String is fine
|
|
||
| @Test | ||
| public void testThatDelegatesIfNoPatternsGiven() { | ||
| RuleBasedRoutingSampler sampler = new RuleBasedRoutingSampler(emptyList(), SPAN_KIND, delegate); |
There was a problem hiding this comment.
Use public API (builder) as much as possible in tests unless specific reason not too. For example, to catch a non-static builder method :)
There was a problem hiding this comment.
I did and I think the end result is much worse :( What was one-liners, now is a longer line plus a 5-line method. Confirms my opinion that builders are oftentimes overrated :(
Co-authored-by: Anuraag Agrawal <[email protected]>
contrib-samplers/src/main/java/io/opentelemetry/contrib/samplers/RuleBasedRoutingSampler.java
Show resolved
Hide resolved
| public RuleBasedRoutingSamplerBuilder drop(AttributeKey<String> attributeKey, String pattern) { | ||
| rules.add( | ||
| new SamplingRule( | ||
| requireNonNull(attributeKey), requireNonNull(pattern), Sampler.alwaysOff())); |
There was a problem hiding this comment.
Put requireNonNull on their own line and add the name parameter. This provides better stack traces. The main reason to have the checks in the first place is to provide a good error experience to the users so we care about those details.
It's why it's not as important in the SamplingRule class since it's an implementation detail. It doesn't hurt, but it's redundant when making sure checks are on the public API, which is what we aim for.
Applies to some of the other methods in this PR too.
contrib-samplers/src/main/java/io/opentelemetry/contrib/samplers/SamplingRule.java
Outdated
Show resolved
Hide resolved
Closes #65
Supersedes #66